Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Surface alerts from trip informed entities #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Surface alerts from trip informed entities #140

wants to merge 1 commit into from

Conversation

cedarbaum
Copy link
Collaborator

Overview

This change surfaces alerts associated with specific trips. It also handles an edge case where the trip entity selector is used for providing alerts for a given route without a tripId.

For example, the MTA buses provide alerts in this form sometimes:

 {
      "id": "MTA NYCT_lmm:planned_work:9387",
      "alert": {
        ...
        "informedEntity": [
          {
            "agencyId": "MTA NYCT",
            "trip": {
              "routeId": "M14D+",
              "directionId": 1
            }
          },
          {
            "agencyId": "MTA NYCT",
            "trip": {
              "routeId": "M14D+",
              "directionId": 0
            }
          }
        ],
       ...
    },
}

In the above case, the alert effectively applies to the whole route since both directions are covered. We therefore consider the following cases for how to handle this:

1.) An alert has an informed trip with no tripId, a routeId, and both directions are specified: consider this alert as affecting the entire route and add a route informed entity.

2.) An alert has an informed trip with no tripId, a routeId, and no directions are specified: consider this alert as affecting the entire route and add a route informed entity.

3.) An alert has an informed trip with no tripId, a routeId and a single direction is specified: consider this alert as affecting the trips running the specified direction.

In all 3 cases, the alert is still considered to affect all trips along the route/direction. Such alerts will now be surfaced from the /trips endpoint, whereas only alerts meeting criteria (1) and (2) are surfaced in the /routes endpoint.

TODOs

  1. This change still does not consider trip start times when matching alerts via a tripId. This will be fine in most cases, but can be incorrect when trips run across multiple days.
  2. Frequency based trips are not handled.

@cedarbaum
Copy link
Collaborator Author

Also @jamespfennell FYI, I think something in CI has been broken recently. I haven't had a chance to look, but things do pass locally. If you have any ideas LMK or else I can also try and debug further.

@jamespfennell
Copy link
Owner

Thanks for the PR and sorry for the slow review! As usual you've put a lot of great thought and research in, thank you!

Some initial thoughts:

  • For cases (1) and (2), should we strip the trip informed entity entirely? It seems like this is case of misusing the API by the alert provider and we could "fix" it.
  • Do you think we should have any of this logic in the gtfs package, instead of here? (Sorry that this has become my standard boring suggestion!)

For the CI - will take a look! The error message is weird and indicates that some dependency of the CI has had its version updated.

@jamespfennell
Copy link
Owner

I know why the CI is broken. Over the last couple of years the Docker team has been merging the Docker compose tool into the main Docker tool (while also rewriting compose in Go). I think now in order to run Docker compose one needs to type docker compose rather than docker-compose. The CI is failing to find the docker-compose tool.

jamespfennell added a commit that referenced this pull request Sep 21, 2024
@jamespfennell
Migrate from docker-compose to docker compose (#140)
5f25408 
This fixes the CI. Unfortunately it requires devs to update their
local Docker setup to use the compose plugin instead of the dedicated
command.
@jamespfennell
Copy link
Owner

CI fixed! Unfortunately running the E2E tests locally via Docker won't work unless you update to use the compose plugin...I haven't done that yet but wanted to unblock the CI at least.

https://docs.docker.com/compose/install/linux/

@cedarbaum
Copy link
Collaborator Author

cedarbaum commented Sep 23, 2024 •
edited
Loading

Thanks for fixing the CI! And it definitely makes sense to do this in the GTFS library, good call!

I've opened a PR there to handle this: jamespfennell/gtfs#18

I can perhaps repurpose this PR to keep the schema change for alert_trip and the /trip API change and then bump the GTFS parser version in another change.

As the current parser change stands, there will be 2 implications for Transiter:

  1. We will continue to persist trip informed entities with empty trip IDs if there is enough other information in the descriptor to uniquely identify the trip (at least in theory). Per the spec, this is allowed in some cases. With my schema change to alert_trip in this PR, we will persist the other required fields (route_id, direction_id, start_time, and start_date) but don't currently make use of them to try and match alerts (only trip ID matches).
  2. There are potential cases both before and after the parser change where a route_id/direction_id combination is provided in an informed entity (outside of the tripID), meaning that the route is affected only in a single direction. Of note, the direction_id is still marked as an experimental field in the spec, but I do believe some GTFS tools attempt to handle such cases today.

    Transiter doesn't currently have a way to map or expose such alerts today, so the current behavior effectively applies the alert to the whole route. I don't think this is likely a huge issue in practice, but something to be aware of. Let me know if you have any thoughts on how to handle such cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamespfennell jamespfennell Awaiting requested review from jamespfennell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cedarbaum @jamespfennell